feat(signing): ship signing/signingtest subpackage + ObserveOnly mode - #479
Conversation
Every consumer writing a handler test that expects signed requests had to reverse-engineer the ~30-line keypair/JWK/StaticJWKSResolver/replay-store pattern in middleware_test.go. signingtest.NewTestAgent + SignAndSend collapse that into a two-line setup and a one-line send. Separately, MiddlewareOptions.ObserveOnly implements the spec's warn_for shadow-mode rollout stop (between supported_for and required_for): verification still runs, but a failing request reaches next.ServeHTTP with no VerifiedSigner in its context instead of getting a 401, and the failure is logged at INFO. A partial or malformed Signature/Signature-Input pair still hard-rejects even under ObserveOnly, per the spec's explicit carve-out that such a pair can't be safely read as signed or unsigned traffic. MIGRATION.md's step-B guidance, which previously described this as a not-yet-landed OnReject shim, is updated to use the real API. Built against adcp/v3/signing — the active module per README's "Modules & versioning" table; adcp/signing (v2) is frozen for security backports only, so this feature does not touch it. Closes adcontextprotocol#53
|
Could you take a look at what happens to the request body when I reproduced this locally with the body Could we make sure a body-size or read failure cannot reach the handler with an incomplete body? Rejecting those failures, or preserving the full stream safely, would address it. A test that checks what the downstream handler actually reads would help cover this case. |
|
Good catch — the reproduction matches exactly. Fix pushed in the follow-up commit (5e7d2d9):
The same fix also covers the analogous case at step 11 (signed request with Generated by Claude Code |
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — no blocking findings.
Reviewed PR #479: additive, spec-anchored ObserveOnly (warn_for shadow mode) plus a new signingtest helper subpackage under adcp/v3/signing/. The change is non-breaking, carries the correct feat(signing) conventional-commit marker in the active adcp/v3 module, and preserves the default fail-closed verification posture. The malformed-header-pair carve-out (CodeHeaderMalformed still hard-rejects even under ObserveOnly) is correct and tested. No schema/generated-type files, TMP signing envelope internals, identity-agent TEE paths, or protocol-managed skills were touched.
Checked against the decision table:
- No critical/high/medium findings (rows 1, 4, 5, 8 do not fire).
- gated_paths is false (row 2 does not fire) despite review_decision=REVIEW_REQUIRED.
- high_risk is false; no deletions/modifications triggers (rows 3, 5).
- No prior decision (row 6 n/a).
- No no-auto-approve team match (row 7 n/a).
Falls through to row 9 → approve.
bokelley
left a comment
bokelley
left a comment
There was a problem hiding this comment.
Maintainer review: ObserveOnly remains fail-closed for malformed authentication headers and does not weaken the default enforcement path; signingtest stays inside the existing adcp/v3 module. Approved pending CI.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass on an additive signing-middleware feature.
This PR adds an opt-in ObserveOnly shadow-mode option to the AdCP request-signing middleware, a new signingtest test-helper subpackage, and matching docs (MIGRATION.md, README.md, doc.go). The reviewer verified the important TMP-signing-adjacent behaviors: fail-closed default, correct CodeHeaderMalformed hard-reject carve-out, no stale VerifiedSigner on pass-through, INFO-level observability, an additive (feat:) wire contract, and thorough test coverage.
Decision-table walk:
- No critical/high findings (row 1 n/a).
- gated_paths is false and review_decision is APPROVED (row 2 n/a).
- high_risk is false; no deleted/modified high-risk entries (rows 3, 5 n/a).
- No medium findings at all (rows 4, 8 n/a).
- No no-auto-approve team match (row 7 n/a).
- Prior decision was already
approve, so row 6 sticky escalation does not apply.
Falls through to row 9 → approve. No inline findings to surface.
3 participants
Closes #53.
Summary
adcp/v3/signing/signingtest—NewTestAgent(t)builds a matched Ed25519*signing.Signer+signing.MiddlewareOptions(a freshStaticJWKSResolverseeded with the signer's public key, a freshNewMemoryReplayStore(0), no revocation).SignAndSend(t, signer, handler, req)signsreq(withCoverContentDigest: true) and delivers it tohandlerin-process viahttptest.NewRecorder, returning the*http.Response. Together these collapse the ~30-line keypair/JWK/resolver/replay-store patternmiddleware_test.gocurrently hand-rolls into two lines of setup + one line to send.MiddlewareOptions.ObserveOnly bool— a real behavioral branch inMiddleware(), not a passthrough field. When true,VerifyRequestSignaturestill runs; on failure the request is logged atslog.LevelInfo(vs the normalslog.LevelWarn) with anobserve_only=trueattribute and passed tonext.ServeHTTPwith noVerifiedSignerin its context — i.e., treated exactly as unsigned. On success, behavior is unchanged.ObserveOnly↔ spec mappingThis maps to the AdCP transport spec's
warn_forrollout stop (supported_for → warn_for → required_for, see Transport capability advertisement). Per spec: "A missing signature or a well-formed signature that fails verification or body binding MUST NOT establish verified-signer identity ... A partial or malformed Signature / Signature-Input pair always hard-rejects."ObserveOnlyimplements that split precisely, not just "log and let everything through":RequiredForop, ...) is observed: INFO log, request passes through unauthenticated.Signature/Signature-Inputheader pair — one header present without the other, or either header present but unparseable (*Error{Code: CodeHeaderMalformed}) — still hard-rejects with 401 even underObserveOnly, because the spec says it "cannot be safely interpreted as either signed or unsigned traffic."adcp/v3/signing/MIGRATION.md's "Step B —warn_for" section previously described this exact mode as not yet landed, with anOnReject-based logs-and-passes-through shim as the interim workaround (and a "delete this before enablingRequiredFor" warning). This PR replaces that shim guidance with the realObserveOnlyAPI throughout the migration guide (bootstrap → step B → common pitfalls → pre-enforcement checklist).Module note
This targets
adcp/v3/signing, notadcp/signing. Per the repo'sREADME.md"Modules & versioning" table andMIGRATING.md,adcp(the pre-v3 module) is frozen at v2.1.1 and receives security backports only —adcp/v3is the actively developed module for AdCP 3.x. A new DX/feature package belongs there, soadcp/signing(the frozen copy) is untouched by this PR.Testing
signingtest's own tests (adcp/v3/signing/signingtest/signingtest_test.go) proveNewTestAgent+SignAndSendproduce a request a realsigning.Middleware-wrapped handler accepts (VerifiedSignerFromContextpopulated, correct algorithm), that the wired verifier is real and not a stub (rejects unsigned traffic on aRequiredForop, rejects a replayed signature), and thatSignAndSendfails fast on a non-absolute request URL (verified via a re-exec'd subprocess, since a subtest'st.Fatalfcan't be observed without failing the parent test/package).middleware_test.gogets three new tests:ObserveOnlylets an unsigned request to aRequiredForoperation through (INFO log, noVerifiedSigner);ObserveOnlylets a well-formed-but-cryptographically-invalid signature through, with a matchingObserveOnly=falsesubtest proving the existing 401 behavior is unchanged; andObserveOnlystill hard-rejects a malformedSignature/Signature-Inputpair (WARN log, 401,WWW-Authenticate: ... request_signature_header_malformed), proving the spec carve-out is real.go build ./...,go vet ./...,gofmt -l, andgolangci-lint run ./signing/...are clean for every file this PR touches. The threestaticcheckfindingsgolangci-lintreports inadcp/v3/signing/jwk.go(deprecatedecdsa.PublicKey.X/.Y/.PrivateKey.Dfield access, Go 1.26) are pre-existing onmainand untouched by this PR.go test ./...passes for both theadcp/v3module (this change) and the root module (unaffected).On item 5 of the issue ("refactor an existing boilerplate-heavy test as a demonstration")
I looked for one and came up empty, honestly reported: the request-signing profile's
StaticJWKSResolver/NewMemoryReplayStoreboilerplate pattern this issue targets appears nowhere else in the repo outsideadcp/signingandadcp/v3/signingthemselves.adcp/v3/webhook's test files (signing_test.go,publisher_test.go,e2e_test.go) use the webhook-signing profile via a different, already-existing dedicated helper (webhookKeypair) and a different verifier entry point (webhook.HTTPHandler, notsigning.Middleware) — not a good refactor target forsigningtest, which is scoped to the request-signing/signing.Middlewarepath the issue describes.I also couldn't refactor
adcp/v3/signing/middleware_test.go's ownTestMiddlewareEndToEndSignAndVerify(the literal ~30-line pattern quoted in the issue) in place: that file ispackage signing(internal, white-box), andsigningtestimportssigning— importingsigningtestfrom insidepackage signingwould be a cyclic import. Instead,signingtest's ownTestNewTestAgentSignAndSendRoundTripreproduces the same scenario (sign, verify via middleware, assert onVerifiedSignerFromContext) using the new two-line + one-line helpers, as the concrete demonstration of the line-count reduction, and the package'sREADME.md/doc.gocross-reference it for discoverability.Test plan
cd adcp/v3 && go build ./... && go vet ./... && go test ./...cd adcp/v3 && golangci-lint run ./signing/...go build ./... && go vet ./... && go test ./...at repo root (unaffected — confirms no accidental frozen-module edits)git statusclean in the working tree aside from the files this PR touches